update management policies with GMP changes #510

lsviben · 2023-07-31T12:44:54Z

Late update for the management policies alpha feature.

Replaces the old managementPolicy to managementPolicies, updates the Importing guide and adds a section for spec.initProvider.

Fixes: #497

netlify · 2023-07-31T12:45:02Z

✅ Deploy Preview for crossplane ready!

Name	Link
🔨 Latest commit	`62f12d6`
🔍 Latest deploy log	https://app.netlify.com/sites/crossplane/deploys/64d1026495520a0008054f43
😎 Deploy Preview	https://deploy-preview-510--crossplane.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

lsviben · 2023-07-31T13:42:13Z

Vale seems to complain for managementPolices, deletionPolicy . Can we add this somewhere for Vale to ignore?

Also btw, I would like to add a guide for using initProvider to ignore changes, but as a separate PR.

plumbis · 2023-07-31T19:00:19Z

Re: Vale -
Yes, you can add them to the crossplane-words.txt file.
https://docs.crossplane.io/contribute/vale/#spelling-errors-and-exceptions

I have some suggested edits for you but before I make them I want to make sure I understand the feature-

These changes came in 1.13?

For LateInitialize the resource will set anything defined in forProvider and import all other settings? It's like half an observe only resource?

Are there any restrictions on what can be combined as a policy (even if it seem to make sense)? Is this chunk from crossplane-runtime the supported set of combinations?

It's suggested to use a combination of managementPolicies without LateInitialize with initProvider to avoid late initialization of fields in forProvider.

I'm not following what this is telling me to do/why to do it.

content/master/concepts/managed-resources.md

lsviben · 2023-08-01T13:07:36Z

Re: Vale - Yes, you can add them to the crossplane-words.txt file. https://docs.crossplane.io/contribute/vale/#spelling-errors-and-exceptions

Thanks. Added them.

I have some suggested edits for you but before I make them I want to make sure I understand the feature-

These changes came in 1.13?

Yeah, these changes are a part of the Granular managmenent policy / ingore changes that came in 1.13, but are actually implemented on a provider level, so they will be released with the next provider release ( for now just the Upbound official ones).

For LateInitialize the resource will set anything defined in forProvider and import all other settings? It's like half an observe only resource?

Late initialization has been around for a while, its not a part of this feature. But its related because this feature now allows us to skip Late Initialization by setting up a granular management policy which does not include it. A big part of the ignore changes feature requests came because Crossplane was late-initializing some fields that users wanted to be left alone.

I saw that Late Initialize section was removed from the docs, probably unintentionally, so I found an old version from the repo and returned it back. Maybe it could do some rework, to avoid confusion on what it actually does.

Are there any restrictions on what can be combined as a policy (even if it seem to make sense)? Is this chunk from crossplane-runtime the supported set of combinations?

yeah we support by default a certain set of policies, which is subject to change if somebody has a good use case. The providers have an option to set their own set of polices to support if they prefer so.

It's suggested to use a combination of managementPolicies without LateInitialize with initProvider to avoid late initialization of fields in forProvider.

I'm not following what this is telling me to do/why to do it.

Reworded, basically it means that in most cases you should use a mangement policy which does not have LateInitialize when using initProviders because the whole point of initProviders is to avoid setting the fields in forProvider. Which can be messed up by Crossplane late-initializing that field in forProvider

plumbis

Thanks for all the work Lovro! There are a lot of suggestions but it's mainly big chunks of moving things around.

Let me know if you have any questions or objections to my suggestions.

content/knowledge-base/guides/import-existing-resources.md

content/master/concepts/managed-resources.md

content/knowledge-base/guides/import-existing-resources.md

content/master/concepts/managed-resources.md

plumbis · 2023-08-01T18:51:06Z

content/master/concepts/managed-resources.md

+The `"Delete"` action replaces the [deletionPolicy]({{<ref "./managed-resources#deletionpolicy" >}})
+field in the managed resource. For now the `deletionPolicy` is still supported,
+but only if it's set to a non default value, which is `Orphan`. If it's set to
+`Orphan`, and the `managementPolicies` is set to `["*"]`, which is default,
+then the external resource will be orphaned when the managed resource is
+deleted. In other cases, non default `managementPolicies` take precedence over
+the `deletionPolicy` field. Keep in mind that this behavior is only applicable
+if the management policy alpha feature is enabled. To sum it up in a table:
+
+{{< table >}}
+| managementPolicies          | deletionPolicy   | result  |
+|-----------------------------|------------------|---------|
+| "*" (default)               | Delete (default) | Delete  |
+| "*" (default)               | Orphan           | Orphan  |
+| contains "Delete"           | Delete (default) | Delete  |
+| contains "Delete"           | Orphan           | Delete  |
+| doesn't contain "Delete"   | Delete (default) | Orphan  |
+| doesn't contain "Delete"   | Orphan           | Orphan  |
+{{< /table >}}


I recommend deleting this section and moving text into the deletionPolicy section, under options, earlier in the doc.
http://docs.crossplane.io/v1.13/concepts/managed-resources/#options

#### Interaction with management policies If a resource configures a Crossplane [management policy](#managementpolicies) the default management policy supports `deletionPolicy: Orphan`. {{< hint "warning" >}} Any management policy using `Delete`, except for the default policy, ignores `deletionPolicy: Orphan`. Crossplane deletes any resources with a `Delete` management policy, even with `deletionPolicy: Orphan` configured. {{< /hint >}}

Removed it and added to the suggested place. But I left the table there, as there is another rule to the ones above:

Crossplane orphans any resources without a `Delete` managment policy, except for the default one, even with `deletionPolicy: Delete` configured.

Seems like much simpler to see it in a table then in words.

plumbis · 2023-08-01T19:44:29Z

content/master/concepts/managed-resources.md

+{{<hint "important" >}}
+The managed resource `initProvider` option is an alpha feature related to 
+[managementPolicies]({{<ref "./managed-resources#managementpolicies" >}}).
+
+Enable the `initProvider` in a provider with `--enable-management-policies`
+in a
+[ControllerConfig]({{<ref "./providers#controller-configuration" >}}).
+{{< /hint >}}
+
+The `spec.initProvider` is a subset of the `spec.forProvider` fields that
+Crossplane merges with `forProvider` to create the external resource, but
+ignores when updating it. This allows users to create a resource with all the
+required fields which may then be controlled externally. For example, a user may
+want to create a `NodeGroup` with the required `desiredSize` field, but then
+let an autoscaler control the field. If the `desiredSize` field is included
+in the`forProvider` field, Crossplane will attempt to update the field, which
+will conflict with the autoscaler.


Suggested restructure to improve wording and flow and add {{<hover>}} support.

Suggested change

{{<hint "important" >}}

The managed resource `initProvider` option is an alpha feature related to

[managementPolicies]({{<ref "./managed-resources#managementpolicies" >}}).

Enable the `initProvider` in a provider with `--enable-management-policies`

in a

[ControllerConfig]({{<ref "./providers#controller-configuration" >}}).

{{< /hint >}}

The `spec.initProvider` is a subset of the `spec.forProvider` fields that

Crossplane merges with `forProvider` to create the external resource, but

ignores when updating it. This allows users to create a resource with all the

required fields which may then be controlled externally. For example, a user may

want to create a `NodeGroup` with the required `desiredSize` field, but then

let an autoscaler control the field. If the `desiredSize` field is included

in the`forProvider` field, Crossplane will attempt to update the field, which

will conflict with the autoscaler.

{{<hint "important" >}}

The managed resource `initProvider` option is an alpha feature related to

[managementPolicies]({{<ref "./managed-resources#managementpolicies" >}}).

Enable the `initProvider` in a provider with `--enable-management-policies`

in a

[ControllerConfig]({{<ref "./providers#controller-configuration" >}}).

{{< /hint >}}

The `spec.initProvider` is a subset of the `spec.forProvider` fields that

Crossplane merges with `forProvider` to create the external resource, but

ignores when updating it. This allows users to create a resource with all the

required fields which may then be controlled externally. For example, a user may

want to create a `NodeGroup` with the required `desiredSize` field, but then

let an autoscaler control the field. If the `desiredSize` field is included

in the`forProvider` field, Crossplane will attempt to update the field, which

will conflict with the autoscaler.

The {{<hover label="initProvider" line="7">}}initProvider{{</hover>}} defines settings Crossplane applies only when creating a new managed resource. Crossplane ignores settings defined in the {{<hover label="initProvider" line="7">}}initProvider{{</hover>}} field that change after creation. {{<hint "note" >}} Settings in `forProvider` are always enforced by Crossplane. Crossplane reverts any changes to a `forProvider` field. Settings in `initProvider` aren't enforced by Crossplane. Crossplane ignores any changes to a `initProvider` field. {{</hint >}} Using `initProvider` is useful for setting initial values that a Provider may automatically change, like an auto scaling group. For example, creating a {{<hover label="initProvider" line="2">}}NodeGroup{{</hover>}} with an initial {{<hover label="initProvider" line="9">}}desiredSize{{</hover>}}. Crossplane doesn't change the {{<hover label="initProvider" line="9">}}desiredSize{{</hover>}} setting back when the Provider auto scales the Node Group. {{< hint "tip" >}} Crossplane recommends configuring {{<hover label="initProvider" line="6">}}managementPolicies{{</hover>}} without `LateInitialize` to avoid conflicts with `initProvider` settings. {{< /hint >}}

Applied, just added a slight change to

Settings in `forProvider` are always enforced by Crossplane. Crossplane reverts any changes to a `forProvider` field. Settings in `initProvider` aren't enforced by Crossplane. Crossplane ignores any changes to a `initProvider` field.

to

Settings in `forProvider` are always enforced by Crossplane. Crossplane reverts any changes to a `forProvider` field in the external resource. Settings in `initProvider` aren't enforced by Crossplane. Crossplane ignores any changes to a `initProvider` field in the external resource.

To make a clear distinction in making changes in managed its external resource

content/master/concepts/managed-resources.md

plumbis

changes to fix vale and improve table display. Otherwise LGTM. Approving, assuming there's no problems with the suggestions.

content/master/concepts/managed-resources.md

Signed-off-by: lsviben <[email protected]>

Signed-off-by: Hasan Turken <[email protected]>

turkenh

Thanks @lsviben, looking good to me!

I just pushed a couple of comments to fix Vale errors and make some improvements in the late initialization section.

@plumbis feel free to have another look, or please let me know if we can hit the merge button.

turkenh · 2023-08-07T13:42:21Z

content/master/concepts/managed-resources.md

+#### Late Initialization
+
+For some of the optional fields, users rely on the default that the cloud
+provider chooses for them. Since Crossplane treats the managed resource as the
+source of the truth, values of those fields need to exist in `spec` of the
+managed resource. In each reconciliation, Crossplane will fill the value of
+a field that's left empty by the user but is assigned a value by the provider.
+For example, there could be two fields like `region` and `availabilityZone` and
+you might want to give only `region` and leave the availability zone to be
+chosen by the cloud provider. In that case, if the provider assigns an
+availability zone, Crossplane gets that value and fills `availabilityZone`. Note
+that if the field is already filled, the controller won't override its value.


I think it needs it's own subsection at least. In my experience, the late initialization behavior is something unexpected for the users, so it would be good to have it mentioned in more detail.

Agreed to have a dedicated section, so that we can refer to it. It would be more important with management policies where users can turn on/off this behavior.

turkenh · 2023-08-07T13:49:53Z

content/master/concepts/managed-resources.md

+#### Late Initialization
+
+For some of the optional fields, users rely on the default that the cloud
+provider chooses for them. Since Crossplane treats the managed resource as the
+source of the truth, values of those fields need to exist in `spec` of the
+managed resource. In each reconciliation, Crossplane will fill the value of
+a field that's left empty by the user but is assigned a value by the provider.
+For example, there could be two fields like `region` and `availabilityZone` and
+you might want to give only `region` and leave the availability zone to be
+chosen by the cloud provider. In that case, if the provider assigns an
+availability zone, Crossplane gets that value and fills `availabilityZone`. Note
+that if the field is already filled, the controller won't override its value.


However, I am confused about that

we have two different sections with the same title

only in the master doc but not the one for v1.13.

turkenh · 2023-08-07T14:05:17Z

content/master/concepts/managed-resources.md

+#### Late Initialization
+
+For some of the optional fields, users rely on the default that the cloud
+provider chooses for them. Since Crossplane treats the managed resource as the
+source of the truth, values of those fields need to exist in `spec` of the
+managed resource. In each reconciliation, Crossplane will fill the value of
+a field that's left empty by the user but is assigned a value by the provider.
+For example, there could be two fields like `region` and `availabilityZone` and
+you might want to give only `region` and leave the availability zone to be
+chosen by the cloud provider. In that case, if the provider assigns an
+availability zone, Crossplane gets that value and fills `availabilityZone`. Note
+that if the field is already filled, the controller won't override its value.


Pushed a commit to fix that.

lsviben force-pushed the gmp branch 3 times, most recently from dbab508 to f060ff3 Compare July 31, 2023 13:33

lsviben requested review from plumbis and turkenh July 31, 2023 13:38

lsviben force-pushed the gmp branch from f060ff3 to 2595d55 Compare July 31, 2023 13:39

turkenh reviewed Aug 1, 2023

View reviewed changes

lsviben requested a review from turkenh August 1, 2023 13:10

lsviben force-pushed the gmp branch from f1c33c5 to 7e91f37 Compare August 1, 2023 14:27

plumbis requested changes Aug 1, 2023

View reviewed changes

lsviben force-pushed the gmp branch from cb870e9 to 32067b1 Compare August 2, 2023 15:04

plumbis approved these changes Aug 4, 2023

View reviewed changes

content/master/concepts/managed-resources.md Outdated Show resolved Hide resolved

content/master/concepts/managed-resources.md Outdated Show resolved Hide resolved

content/master/concepts/managed-resources.md Outdated Show resolved Hide resolved

lsviben added 8 commits August 6, 2023 16:12

update management policies with GMP changes

005db47

Signed-off-by: lsviben <[email protected]>

address comments

a7ff8bd

Signed-off-by: lsviben <[email protected]>

add a few words to vale dictionary

032d138

Signed-off-by: lsviben <[email protected]>

fix vale

4b5864e

Signed-off-by: lsviben <[email protected]>

rewording, moving stuff based on review suggestions

78b728d

Signed-off-by: lsviben <[email protected]>

fix Vale

cdfe15d

Signed-off-by: lsviben <[email protected]>

add css for dark mode tables

73c3d1a

Signed-off-by: lsviben <[email protected]>

applied suggestions from review

46ebbb1

Signed-off-by: lsviben <[email protected]>

lsviben force-pushed the gmp branch from 5d1c67b to 46ebbb1 Compare August 6, 2023 14:15

turkenh added 2 commits August 7, 2023 16:34

Fix Vale by not breaking references in import existing resources

fbe86e6

Signed-off-by: Hasan Turken <[email protected]>

Remove duplicate late init sections & sync v1.13 with master

cc084ec

Signed-off-by: Hasan Turken <[email protected]>

turkenh force-pushed the gmp branch from 9038857 to 5ca93d9 Compare August 7, 2023 14:33

Fix all vale errors for management policies

62f12d6

Signed-off-by: Hasan Turken <[email protected]>

turkenh force-pushed the gmp branch from 5ca93d9 to 62f12d6 Compare August 7, 2023 14:40

turkenh approved these changes Aug 7, 2023

View reviewed changes

plumbis merged commit 7ac4dc6 into crossplane:master Aug 7, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update management policies with GMP changes #510

update management policies with GMP changes #510

lsviben commented Jul 31, 2023 •

edited

Loading

netlify bot commented Jul 31, 2023 •

edited

Loading

lsviben commented Jul 31, 2023

plumbis commented Jul 31, 2023

lsviben commented Aug 1, 2023

plumbis left a comment

plumbis Aug 1, 2023

lsviben Aug 2, 2023

plumbis Aug 1, 2023

lsviben Aug 2, 2023

plumbis left a comment

turkenh left a comment

turkenh Aug 7, 2023

turkenh Aug 7, 2023

turkenh Aug 7, 2023

update management policies with GMP changes #510

update management policies with GMP changes #510

Conversation

lsviben commented Jul 31, 2023 • edited Loading

netlify bot commented Jul 31, 2023 • edited Loading

✅ Deploy Preview for crossplane ready!

lsviben commented Jul 31, 2023

plumbis commented Jul 31, 2023

lsviben commented Aug 1, 2023

plumbis left a comment

Choose a reason for hiding this comment

plumbis Aug 1, 2023

Choose a reason for hiding this comment

lsviben Aug 2, 2023

Choose a reason for hiding this comment

plumbis Aug 1, 2023

Choose a reason for hiding this comment

lsviben Aug 2, 2023

Choose a reason for hiding this comment

plumbis left a comment

Choose a reason for hiding this comment

turkenh left a comment

Choose a reason for hiding this comment

turkenh Aug 7, 2023

Choose a reason for hiding this comment

turkenh Aug 7, 2023

Choose a reason for hiding this comment

turkenh Aug 7, 2023

Choose a reason for hiding this comment

lsviben commented Jul 31, 2023 •

edited

Loading

netlify bot commented Jul 31, 2023 •

edited

Loading